Using on_despawn here means that this won't trigger if the component is first removed, and then the entity is despawned. I think that would put the world in a weird state where resource_entities() references a despawned entity, so you can never create a new one or remove the old one.

Is the reason that we don't clear it in on_remove because we want to re-use the Entity if the resource is removed and recreated?

Would it work to have on_add_hook check whether original_entity has been despawned, and treat that the same as no entry in resource_entities? I think that would make the hooks treat a remove followed by a despawn the same as a direct despawn. (Hmm, and at that point you might not even need the separate on_despawn hook, although it has the advantage of doing warn! closer to the bad code.)

I considered this edge case (first removing, then despawning), and this does create an invalid state. I decided that it was a rare enough interaction to not worry about it. I assume that a vast majority of users will still use all of the usual world methods to interact with resources and those methods never despawn a resource entity.

The reason we don't clear it is indeed to keep the resource entity stable. This is also how components-as-entities is going to work, so it fits in with future plans (although with components-as-entities, we'd spawn the entity on registration instead of first-insert).

I think the original_entity despawn-check would work (I keep forgetting that entities come with version numbers in the generation), although at that point I think I prefer panicking: You're never supposed to be in a situation where ResourceCache has an entity that is not spawned.

at that point I think I prefer panicking: You're never supposed to be in a situation where ResourceCache has an entity that is not spawned.

Yeah, panicking in an invalid situation seems fine! That will give us a place to tell the user what went wrong and how to fix it. (Or maybe we should invoke the error handler, in case the user configures it to never panic?)

... Oh, does this approach mean that resources won't work with DespawnOnExit? I bet we will want state-scoped resources eventually.

Why did these tests get removed? The behavior they're checking seems important to preserve.

I think deleting the filtered_resource_mut_conflicts_write_with_resmut test was a mistake and I deleted filtered_resource_reflect because I had a hard time understanding the test and since ReflectResource was changed so much I didn't know how to fix it.

I'll look into this again.

Huh, I thought this got rolled into ComponentTicks in #21562. ... Ah, it added changed_by it to a bunch of the Ticks structs, but not that one. I think we should add a ComponentTicks::changed_by field so that we don't need this method, but that shouldn't be part of this PR.

The old code wouldn't call R::from_world if the resource already exists, while the new code is calling it unconditionally and relying on InsertMode::Keep. I think we want to avoid running the initializer unnecessarily by checking entity_mut.contains_id(resource_id) first.

Why check for IsResource here? The hooks should prevent duplicates, and the resource_entities map is the authoritative source of whether this was a "resource entity" or not.

... ah, it's because we need that check in Res parameters so that they don't conflict with queries that use Without<IsResource> default query filters. ... oh, but you aren't actually checking that in UnsafeWorldCell::get_resource_with_ticks!

I think you need to check for the IsResource component consistently. Otherwise a malicious user could remove the component, and then get aliasing between queries with Without<IsResource> and resource access APIs.

It might help to have a method like UnsafeWorldCell::get_resource_entity(self, component_id: ComponentId) -> Option<UnsafeEntityCell> that will encapsulate doing the lookup on resource_entities and checking that the entity exists and has the component_id and IsResource components.

We may also want to cache the component_id of IsResource on the World somehow, or reserve a fixed value, in order to avoid the hash lookup from the TypeId.

... a malicious user could ...

While all of the things you say are true: this is a game engine. Our users are game developers, not hackers. I'll look into this, but we can't guardrail every single corner of Bevy.

While all of the things you say are true: this is a game engine. Our users are game developers, not hackers. I'll look into this, but we can't guardrail every single corner of Bevy.

Hah! Fair :). Though maybe I should apply Hanlon's razor and say that a foolish user could do those things :).

If the consequences here were a panic or weird behavior then I wouldn't worry, but this can cause Undefined Behavior if we get it wrong, and I think Bevy is usually pretty careful not to allow UB in safe code.

I think it's really just the checks in Res and ResMut that are important for safety, though, since those need to not alias with our friend Query<EntityMut, Without<IsResource>>. But if we change those, then it would just be surprising if World::get_resource found a resource that Res did not.

A hypothetical future implementation of Res<R> in terms of Single<R, With<IsResource>> would also check for IsResource, of course.

Hmm, another possibility for future work here might be to support immutable resources by removing this bound and adding Resource<Mutability = Mutable> to things like ResMut and World::resource_mut. (But not in this PR, of course!)

There are so many component things you can implement on resources once this PR is through, just by selectively allowing derives that already exists for components:

#[derive(Resource)]
#[require(B, C)]
#[relationship(relationship_target = Children)]
#[resource(clone_behavior = Ignore, storage = "SparseSet", immutable)]
struct A;

Maybe not all of these make sense (I'm personally sceptical about relationships), but a lot of them would, and we get a lot of control in how we enable them (and what error messages are appropriate for when it doesn't work).

I think you can remove the SAFETY comments here (and below for ResMut), since this check is entirely safe code now!

Not quite! resource_entities() is unsafe for UnsafeWorldCell, like getting most metadata from UnsafeWorldCell.

Not quite! resource_entities() is unsafe for UnsafeWorldCell, like getting most metadata from UnsafeWorldCell.

Oh, I see!

... In that case, I think we're missing an unsafe block around it. (I wish we had unsafe_op_in_unsafe_fn enabled.)

To confirm the design tradeoff here: The main reason to exclude resources by default is that Query<EntityMut> will conflict with Res<R> parameters.

And that includes things like Query<EntityMut, With<Player>>, because we don't have anything like archetype invariants that lets Bevy assume a player is not a resource.

The cost is that this will make it hard to query for resources (#21363). Query<&R> will not find the entity with an R resource, because the query only mentions R and not IsResource!

There was some recent discussion about this on Discord arguing that we should not do this, meaning users would need to write Query<EntityMut, (With<Player>, Without<IsResource>)>. I'm personally starting to be convinced, since I think it does help keep the mental model simpler, and I don't expect beginners to run into Query<EntityMut>. But I see the advantages to either approach!

And that includes things like Query<EntityMut, With>, because we don't have anything like archetype invariants that lets Bevy assume a player is not a resource.

Someone should really get on that 😉 .

Given the amount of buzz on Discord, I'm going to do a somewhat specific analysis about the two scenarios: IsResource as a default-query-filter and IsResource as a normal marker component.

Why remove this method? You left the typed fn register_resource<R>() method.

If we want to remove it in favor of calling register_component_with_descriptor(), I think we should leave it #[deprecated] for a release and mention it in the migration guide, and we should also remove register_resource<R>() for consistency.

The reason why I removed register_resource_with_descriptor is that it didn't really make sense to have it around anymore, because resources are now very hard-coded components (see macros/src/component.rs). They only exist with one particular ComponentDescriptor, so it wouldn't make sense to allow users to register custom resources.

This is the reason why register_resource<R>() still exists while register_resource_with_descriptor doesn't.

The reason why I removed register_resource_with_descriptor is that it didn't really make sense to have it around anymore, because resources are now very hard-coded components (see macros/src/component.rs). They only exist with one particular ComponentDescriptor, so it wouldn't make sense to allow users to register custom resources.

Huh! I had been assuming that we needed to continue supporting dynamic resources. I suppose if resources are just special components then we can tell users to just use dynamic components, though.

If we do drop support for dynamic resources, we probably need to mention that more explicitly in the migration guide, though!

I remember that #21346 had a way to re-use these hooks for dynamic resources, but in this version it looks like they're only available on the Resource trait. Do we want to make these free functions and try to plug them in when registering a dynamic resource?

For that matter, even if we don't plug them in automatically for dynamic resources, these don't seem to depend on the Self type and can be free functions instead of associated functions on the trait.

I followed the example of the Relationship trait, which keeps the hooks under the trait. I don't see the use of them being free functions, as I can plug them into the dynamic resource as-is through Resource::on_add_hook, I just like to organize them on the trait they're related to.

I don't see the use of them being free functions

One reason is to make them more convenient to use for dynamic resources.

Another is to avoid monomorphization. Putting them on the trait means they get compiled separately for every Resource type, while a non-generic free function would only be compiled once.

In contrast, the Relationship trait uses things like Self::RelationshipTarget, so it really does need to be compiled separately for each relationship.